refactor: Add BuildString to rattler_conda_types#2430
Conversation
f169025 to
9c76043
Compare
BuildString and BuildStringPrefix to rattler_conda_typesBuildString to rattler_conda_types
df53350 to
1cd9c5e
Compare
67236c8 to
e6ee71a
Compare
|
Sadly still some merge conflicts |
|
|
||
| fn build(&self) -> &str { | ||
| &self.repodata_record.package_record.build | ||
| fn build(&self) -> Option<&BuildString> { |
There was a problem hiding this comment.
I think I dislike Option here - it would be better if BuildString had a special empty state internally, no? Then the hash implementation can also easily be part of the type etc?
Adds a newtype in rattler_conda_types::package wrapping a String. The validating constructor BuildString::new enforces the CEP26 character set (ASCII alphanumeric plus '_', '.', '+') and a 64-byte maximum length; BuildString::new_unchecked accepts any value as-is. The internal structure of the build string (prefix, hash, build number) is intentionally not exposed: callers treat the value as a single opaque token. Use BuildString::append / ::prepend (validating) or their _unchecked siblings to build composite values. Construction: - BuildString::new(value) -> Result (CEP26 validation) - BuildString::new_unchecked(value) -> Self (infallible) Mutation: - append / prepend(other) -> Result (validating) - append_unchecked / prepend_unchecked(other) Hash derives from the inner String, so source-identifier hashes in lockfiles keyed by build strings stay stable when an existing String field is migrated to BuildString. PartialEq against str / &str / String is provided so comparison call sites work without conversion.
Replaces String-typed build strings on the core records with the dedicated newtype: - PackageRecord::build, IndexJson::build, MinimalPrefixRecord::build, GenericVirtualPackage::build_string: String -> BuildString. HasArtifactIdentificationRefs::build now returns &BuildString; lockfile v5/v6/v7 conda models and v7 source models store Cow<'a, BuildString> instead of Cow<'a, str>. PackageRecord::new takes a BuildString directly so callers pick the strictness explicitly via BuildString::strict / ::unchecked. All call sites across rattler, rattler-bin, rattler_cache, rattler_index, rattler_lock, rattler_repodata_gateway, rattler_solve, rattler_virtual_packages, py-rattler and js-rattler are updated to match.
…entry points Address the second round of review feedback that arrived after the first refactor commit was pushed. - BuildString::append/prepend now also reject empty results, restoring the [1, 64]-byte invariant for combined values. Adds BuildStringError::Empty. - Drop the unused append_unchecked/prepend_unchecked variants; production code only ever needs the validating versions. - py-rattler PackageRecord(...) constructor (PyRecord::create) now returns PyResult and raises ValueError on invalid build strings, matching the set_build setter. - js-rattler PackageRecord JSON constructor and the simple_solve locked- packages path validate build strings via BuildString::new instead of going through the permissive serde / new_unchecked entry points. - Test fixtures that depended on the permissive behaviour migrated from py3-none-any / cp39-cp39-linux_x86_64 to underscore-separated build strings.
Makes "BuildString may be empty" expressible, but then it was via the Option before.
| Ok(()) | ||
| } | ||
|
|
||
| fn validate(value: &str) -> Result<(), BuildStringError> { |
There was a problem hiding this comment.
According to the CEP BuildStrings cannot be empty. That means we cant have a default implementation for build string.
There was a problem hiding this comment.
I think we should have something like "unset" or "none" as the default value.
| record.name.as_normalized(), | ||
| record.version, | ||
| ), | ||
| None => format!("{}-{}.json", record.name.as_normalized(), record.version), |
There was a problem hiding this comment.
This is definitely invalid as it breaks assumptions with two - in these strings. We should better define some kind of placeholder empty state I think, like "empty".
| Ok(()) | ||
| } | ||
|
|
||
| fn validate(value: &str) -> Result<(), BuildStringError> { |
There was a problem hiding this comment.
I think we should have something like "unset" or "none" as the default value.
Description
... and use those in a second step.
How Has This Been Tested?
Unit tests: All existing tests pass unchanged.
AI Disclosure
Tools: Claude
Checklist: